Skip to content

Conversation

@pierrepetersmeier
Copy link
Contributor

Resolves #1343

@pierrepetersmeier pierrepetersmeier self-assigned this Jun 26, 2025
@danielfeismann danielfeismann added enhancement New feature or request labels Jun 26, 2025
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get to all of it. Just a first view thoughts that imho needs to be adapted. As well it might get easier since it would reduce the amount of changes.

@danielfeismann danielfeismann added this to the Version 8.2 milestone Jul 25, 2025
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks already quite ok, but I guess some parts can be improved.

Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok we're getting closer and by the way exploited another issue, which make me stopping my review within the tests. But please start alreay with my comments below

@pierrepetersmeier pierrepetersmeier force-pushed the pp/#1343-add-ground-temperature-1m-as-option-to-weather-data branch from b09a3ba to aa481ca Compare November 6, 2025 13:51
Comment on lines +11 to +12
ground_temperature_level_1 double precision,
ground_temperature_level_2 double precision,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please align the format here

Comment on lines +10 to +11
tg_1 double precision,
tg_2 double precision,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please name them here same as in cosmo. Or is there a reason not to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other entries also have a different naming strategy than in Cosmo.

Comment on lines +82 to +83
Optional.of(Quantities.getQuantity(278.019012451172d, StandardUnits.TEMPERATURE)),
Optional.of(Quantities.getQuantity(278.019012451172d, StandardUnits.TEMPERATURE))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. see #1470

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look on my proposal in #1470 and adapt the check of the expected values the same way (split into model.time == expected.time, and same for .values)

UUID.fromString("a4bbcb77-b9d0-4b88-92be-b9a14a3e332b"),
ColumnScheme.ENERGY_PRICE
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. what happens if we only get one ground temp and not two?

pierrepetersmeier and others added 3 commits November 11, 2025 13:15
Co-authored-by: Daniel Feismann <98817556+danielfeismann@users.noreply.github.com>
Co-authored-by: Daniel Feismann <98817556+danielfeismann@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ground temperature (1m) as option to weather data

3 participants